Revert mutation observer for forms #396
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Emanuele Feliziani <[email protected]>
Signed-off-by: Emanuele Feliziani <[email protected]>
GioSensation
commented
Oct 10, 2023
| this.formAnalyzer = new FormAnalyzer(this.form, input, this.matching) | ||
| this.recategorizeAllInputs() | ||
| return this | ||
| } |
There was a problem hiding this comment.
This is what was causing the problem. Basically, in certain edge cases the container of the field (what here we call this.form) could become the body. When that happened, any new field was hitting this conditional thus dropping everything and starting from scratch. We were doing this for every single added field, which in Asana could be hundreds at a time as you scroll through large projects. This wasn't always hitting the failsafes because we kept destroying and re-building the form data.
The failsafe we built for the form-specific mutation observer wasn't running here because the trigger comes from outside (the scanner), not from the internal observer.
In our browser the slowdown was limited (still massive), but on Firefox it could hang for several seconds and show the warning.
GioSensation
commented
Oct 10, 2023
| foundForm.addInput(input) | ||
| } else { | ||
| this.stopScanner('The form has too many inputs, destroying.') | ||
| } |
There was a problem hiding this comment.
I've moved this check in the scanner as well for added security. I have a few follow up tasks in Asana to try and understand this flow better and see how we can make it more resilient to breakage.
GioSensation
commented
Oct 10, 2023
| @@ -8,7 +8,7 @@ import {test as base} from '@playwright/test' | |||
| */ | |||
| const test = base.extend({}) | |||
|
|
|||
| test.describe('Mutating form page', () => { | |||
| test.describe.skip('Mutating form page', () => { | |||
There was a problem hiding this comment.
Just switching the test off for now.
GioSensation
commented
Oct 10, 2023
| } | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
This wasn't directly related to the breakage, but I'm dropping it now before investigating things further in the next few days.
shakyShane
approved these changes
Oct 10, 2023
This was referenced Oct 10, 2023
CDRussell
pushed a commit
to duckduckgo/Android
that referenced
this pull request
Oct 11, 2023
Task/Issue URL: https://app.asana.com/0/1205691418983700/1205691418983700 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: GioSensation <[email protected]>
graeme
added a commit
to duckduckgo/macos-browser
that referenced
this pull request
Oct 11, 2023
Task/Issue URL: https://app.asana.com/0/1205691418983706/1205691418983706 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 BSK PR: duckduckgo/BrowserServicesKit#530 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). --------- Co-authored-by: GioSensation <[email protected]> Co-authored-by: Graeme Arthur <[email protected]>
joshliebe
pushed a commit
to duckduckgo/Android
that referenced
this pull request
Nov 7, 2023
Task/Issue URL: https://app.asana.com/0/1205691418983700/1205691418983700 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2 ## Description Updates Autofill to version [8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2). ### Autofill 8.4.2 release notes ## What's Changed * Ema/perf followups by @GioSensation in duckduckgo/duckduckgo-autofill#386 * Fix handler regression by @GioSensation in duckduckgo/duckduckgo-autofill#392 * Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in duckduckgo/duckduckgo-autofill#388 * Revert mutation observer for forms by @GioSensation in duckduckgo/duckduckgo-autofill#396 **Full Changelog**: duckduckgo/duckduckgo-autofill@8.4.1...8.4.2 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: GioSensation <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewer: @shakyShane
Asana: https://app.asana.com/0/0/1205690199450910/f
CC: @kzar
Description
Reverts the code that observes for form changes. This was causing the script to spin out of control in certain cases. To clarify, it wasn't the observer itself, but the way we were re-scanning the page when new fields were added (added in version 8.1.0). This is just a patch to stop the bleeding, I will follow up with a more comprehensive set of fixes as roughly outlined here.
Steps to test
To repro the issue:
With this version it should not trigger the warning or freeze the page.